Why bitfields instead of one uint16_t and two uint8_t?

No special reasons for using bitfields just it's easier to keep the structure word aligned.
Will be updated with uint16_t and uint8_t, thanks.

this seems only to be used for logging, maybe use dev->name instead. That's how it's done at other drivers

use DT_INST_PROP_OR(n, local_mac_address, {0}) instead

move these in the DT_INST_FOREACH_STATUS_OKAY at the end

please only do DT_INST_FOREACH_STATUS_OKAY() one per driver

if dev_data->iface is no longer NULL iface init was done, no need to have a seperate var in dev_data

it's part of a staric struct, its initial value is 0 / false.
also dev_data->link_up seems unnecessary, as the phy is responsible for not notifying the ethernet driver multiple times. dev_data->link_up can be removed IMO

line 34: #define GMAC_QUEUE_NUM DT_INST_PROP(0, num_queues)
still uses the first instance, it should not, as it is used for the other instances too

same here

line 1663. #if DT_INST_NODE_HAS_PROP(0, mac_eeprom) too
and line 1668

line 1006: switch (DT_INST_ENUM_IDX(0, phy_connection_type)) { too

no need to check for local address, also a invalid mac address should not stop the init as the user can change it later via the api.

OK, remove them and only handle mac_eeprom and zephyr_random_mac_address cases.
Updated, thanks a lot.

is this part documented in the datasheet or other documentation of the soc manufacturer?
If not it should be removed, as it's up to the user to to set the mac later f.e. via the set_config api.

Also having ETH_SAM_GMAC_MAC_I2C_INT_ADDRESS and ETH_SAM_GMAC_MAC_I2C_INT_ADDRESS_SIZE as a Kconfig is not a good idea, as it can this way only be set for all instances at one, so for every instance another eeprom has to be used.

Getting MAC address is not a part of the datasheet or documents for the soc.
The consideration of keeping the function here is mainly for compatible with the code without applying this patch.

ETH_SAM_GMAC_MAC_I2C_INT_ADDRESS and ETH_SAM_GMAC_MAC_I2C_INT_ADDRESS are used for all the instance which has mac-eeprom. It assumes that all the instances get MAC address starting at the same EEPROM offset address. The MAC address could be different for each instance with dts like bellow:

&gmac0 {
	mac-eeprom = <&eeprom0>;
	...
};
&gmac1 {
	mac-eeprom = <&eeprom1>;
	...
};

if it's not documented in a datasheet or a document for the soc, it should be removed, we had a similar case here
#87953 (comment)

Thanks, updated with the following:

• remove the getting MAC address from EEPROM from the GMAC driver
• remove the related configurations, properties from defconfig, device tree and yaml
• use zephyr,random-mac-address; for MAC address initialize

Hi @TonyHan11 ,

I want to continue to use the eeprom mac address and I don't want to use an random mac.
What is the alternative ?

setting the mac during runtime directly via 'set_config' or the 'net_mgmt' api

What sample do that ?

static int set_mac_addr(struct net_if *iface, uint8_t *mac_addr)
{
    if (iface == NULL) {
        LOG_ERR("No interface configured");
        return -EINVAL;
    }
    struct ethernet_req_params params = {0};

    memcpy(params.mac_address.addr, mac_addr, 6);

    net_if_down(iface);

    int ret = net_mgmt(NET_REQUEST_ETHERNET_SET_MAC_ADDRESS, iface, &params,
               sizeof(struct ethernet_req_params));
    if (ret < 0) {
        LOG_ERR("changing mac address failed with error code %d", ret);
        goto set_mac_addr_end;
    }

    ret = memcmp(net_if_get_link_addr(iface)->addr, mac_addr, sizeof(params.mac_address.addr));
    if (ret != 0) {
        LOG_ERR("changing mac address failed - set mac not equal to provided mac %d", ret);
        ret = -1;
        goto set_mac_addr_end;
    }
    LOG_DBG("MAC address changed to %02x:%02x:%02x:%02x:%02x:%02x",
        mac_addr[0], mac_addr[1], mac_addr[2], mac_addr[3], mac_addr[4], mac_addr[5]);

set_mac_addr_end:
    net_if_up(iface);

    return ret;
}

We use this before using dhcp, so with a SYS_INIT with prio 94

I don't remember a sample, that shows that

I don't remember a sample, that shows that

It is because we don't have this in Zephyr yet and here the driver do the bootstrap to the user.

I think what you mention is a great candidate to be automatically applied based on configuration, for instance:

(1) local-mac-address from devicetree
(2) Random address
(3) Eeprom address
(4) None

The network management could do this to the user as a bootstrap and then we can remove this from the driver. User still have the power to complete skip when no option is provided.

In my case, I need to test sometimes on many boards and it is unacceptable to me change ip address each time I change a board. Problems could happen if the random MAC is the same between boards.

only these sources for mac addresses should be in a ethernet driver:

1. local-mac-address from devicetree
2. random address
3. a kind of unique address generated via HWINFO
4. If specified and documented by the manufacturer other sources like special registers or fuses (user programmable or set by the manufacture)

For production devices you are supposed to use the api to set the mac address, if 4 is not available.

Adding other generic methods into the drivers would lead to duplicated code and makes the drivers more messy and less maintainable.

Hi @maass-hamburg,

I think we want the same. I'm fine with your comments but I want a solution to use the EEPROM which user don't need to act. The implementation is on this driver for years and there is no current proposal to solve what you don't like, which is fine.

So, when a viable proposal is defined the EEPROM read can be removed from the driver otherwise it should be kept.

NOTE: User can chose to not have Network Management and this imply that solution should be inside the driver somehow. One possible proposal is create a some hook mechanism that allows a generic way to apply the EEPROM MAC to any platform or anything else. HWINFO should not be a mandatory dependency to solve this. Inclusive, a hook could even use the HWINFO and this do not need to be inside the driver.

Well, then let's deprecate CONFIG_ETH_SAM_GMAC_MAC_I2C_EEPROM (add a select DEPRECATED) and only make the dt prop mac-eeprom work, when there is only one activated instance. ( a build assert with !(DT_ANY_INST_HAS_PROP_STATUS_OKAY(mac_eeprom) && ( DT_NUM_INST_STATUS_OKAY(DT_DRV_COMPAT) > 1)))

Agreed!

Updated with the following:

• revert the code related with mac-eeprom
• add deprecated to mac-eeprom and ETH_SAM_GMAC_MAC_I2C_EEPROM
• add BUILD_ASSERT to limited mac-eeprom be used when there's only one activated GMAC instance

COND_CODE_1 is not needed

line 34:

#define GMAC_QUEUE_NUM DT_INST_PROP(0, num_queues)

still depends on the first instance

can probably use something like DT_INST_FOREACH_PROP_ELEM() to iterate over the interrupts or interrupt-names. (Also use DT_INST_IRQN_BY_IDX()) Also looks like num-queues is just the amount of interrupts, if thats the case num-queues` is redundant and should be removed.